Okta SCEP endpoint #34721
Okta SCEP endpoint #34721
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #34721 +/- ##
==========================================
+ Coverage 66.09% 66.16% +0.06%
==========================================
Files 2080 2085 +5
Lines 176330 176367 +37
Branches 7166 7137 -29
==========================================
+ Hits 116554 116689 +135
+ Misses 49069 48947 -122
- Partials 10707 10731 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # server/datastore/mysql/hosts.go # server/datastore/mysql/hosts_test.go # server/datastore/mysql/schema.sql
# Conflicts: # server/datastore/mysql/schema.sql
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR introduces Conditional Access SCEP certificate enrollment support for Fleet, enabling secure certificate issuance for third-party MDM integrations. The implementation includes a new SCEP service, MySQL-backed certificate depot, database schema for tracking certificates and serials, and comprehensive integration tests with rate limiting and enrollment secret verification. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SCEP as SCEP Handler
participant Middleware as Challenge Middleware
participant Depot
participant DB
Client->>SCEP: GetCACaps()
SCEP->>DB: Retrieve CA cert/key
SCEP-->>Client: SHA-256, AES, POSTPKIOperation<br/>(renewal not supported)
Client->>SCEP: GetCACert()
SCEP->>DB: Retrieve CA certificate
SCEP-->>Client: CA certificate (DER)
Client->>SCEP: PKIOperation(CSR with challenge)
Note over SCEP: Decrypt SCEP envelope
SCEP->>Middleware: challengeMiddleware validates<br/>enrollment secret
alt Challenge Valid
Middleware->>Depot: Sign CSR
Depot->>Depot: Extract UUID from SAN URI
Depot->>DB: Check host exists
Depot->>DB: Check rate limit<br/>(enroll cooldown)
alt Rate Limit Exceeded
Depot-->>SCEP: RateLimitError
SCEP-->>Client: HTTP 429
else Rate Limit OK
Depot->>DB: Insert certificate<br/>+ serial
Depot-->>SCEP: Signed certificate
SCEP-->>Client: Certificate (SCEP envelope)
end
else Challenge Invalid / Missing UUID
Middleware-->>SCEP: PKI Failure
SCEP-->>Client: PKI Failure response<br/>(no certificate issued)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
server/datastore/mysql/hosts_test.go (1)
8479-8489: Cover cleanup verification for conditional_access_scep_certificates.Good insertion to exercise CA SCEP, but if conditional_access_scep_certificates isn’t included in hostRefs, the test won’t assert it’s removed by DeleteHosts. Add the table to hostRefs, or add targeted pre/post asserts here to guarantee coverage.
Suggested minimal assertions:
@@ _, err = ds.writer(context.Background()).Exec(` INSERT INTO conditional_access_scep_certificates (serial, host_id, name, not_valid_before, not_valid_after, certificate_pem, revoked) VALUES (?, ?, ?, ?, ?, ?, ?) `, caCertSerial, host.ID, "test-ca-host", time.Now().Add(-1*time.Hour), time.Now().Add(24*time.Hour), "-----BEGIN CERTIFICATE-----", false) require.NoError(t, err) + + // Assert CA SCEP row exists pre-delete + var caCount int + err = ds.writer(context.Background()).Get(&caCount, `SELECT COUNT(1) FROM conditional_access_scep_certificates WHERE host_id = ?`, host.ID) + require.NoError(t, err) + require.Equal(t, 1, caCount) @@ err = ds.DeleteHosts(context.Background(), []uint{host.ID}) require.NoError(t, err) @@ // Check that all the associated tables were cleaned up. + // Assert CA SCEP row removed post-delete + err = ds.writer(context.Background()).Get(&caCount, `SELECT COUNT(1) FROM conditional_access_scep_certificates WHERE host_id = ?`, host.ID) + require.True(t, err == nil || errors.Is(err, sql.ErrNoRows)) + require.Zero(t, caCount)Also, please verify conditional_access_scep_certificates is listed in hostRefs so it’s covered by the generic loops. If it is, the direct asserts are optional but still helpful as a focused guard.
ee/server/integrationtest/condaccess/scep_rate_limit_test.go (2)
27-29: Consider adding hosts table cleanup.The deferred cleanup only truncates the
conditional_access_scep_*tables, but the test creates hosts (lines 39-47, 61-69) that may not be cleaned up. If theBaseSuitedoesn't automatically clean thehoststable, consider adding it to the truncation list to prevent test data leakage.defer mysql.TruncateTables(t, s.BaseSuite.DS, []string{ - "conditional_access_scep_serials", "conditional_access_scep_certificates", + "conditional_access_scep_serials", "conditional_access_scep_certificates", "hosts", }...)
20-74: Consider testing cooldown expiration.The test configures a 5-minute cooldown (line 22) and verifies immediate requests are rate-limited, but doesn't verify that requests succeed after the cooldown period expires. Consider adding a test case that:
- Manipulates the certificate's
created_attimestamp to simulate cooldown expiration- Verifies the same host can successfully request a new certificate after cooldown
This would provide more comprehensive coverage of the rate-limiting behavior.
server/datastore/mysql/conditional_access_scep_test.go (1)
189-193: Tighten the negative-path assertionRight now the missing-UUID case only probes serial
1to assert nothing was persisted. That will miss a regression if the table’s auto-increment ever skips over1(e.g. whenTRUNCATEis not used) and we mistakenly store a certificate under serial2+. It’d be safer to assert by host—for example, callGetConditionalAccessCertCreatedAtByHostIDfor the host under test and expect a not-found error—so we fail regardless of the next serial value.server/datastore/mysql/conditional_access_scep.go (1)
16-20: Consider adding LIMIT 1 for defensive coding and explicit intent.While certificate serial numbers should be unique and
sqlx.GetContextwill error on multiple rows, addingLIMIT 1makes the uniqueness expectation explicit and guards against potential schema issues or bugs.Apply this diff:
stmt := ` SELECT host_id FROM conditional_access_scep_certificates WHERE serial = ? AND revoked = 0 AND not_valid_after > NOW() + LIMIT 1 `
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cmd/fleet/serve.go(2 hunks)ee/server/integrationtest/condaccess/condaccess_test.go(1 hunks)ee/server/integrationtest/condaccess/scep_rate_limit_test.go(1 hunks)ee/server/integrationtest/condaccess/suite.go(1 hunks)ee/server/service/condaccess/config.go(1 hunks)ee/server/service/condaccess/config_test.go(1 hunks)ee/server/service/condaccess/depot/depot.go(1 hunks)ee/server/service/condaccess/scep.go(1 hunks)server/datastore/mysql/conditional_access_scep.go(1 hunks)server/datastore/mysql/conditional_access_scep_test.go(1 hunks)server/datastore/mysql/hosts.go(1 hunks)server/datastore/mysql/hosts_test.go(1 hunks)server/datastore/mysql/migrations/tables/20251103000000_AddConditionalAccessSCEPTables.go(1 hunks)server/datastore/mysql/mysql.go(2 hunks)server/datastore/mysql/schema.sql(2 hunks)server/fleet/datastore.go(1 hunks)server/fleet/mdm.go(1 hunks)server/mock/datastore_mock.go(3 hunks)server/service/testing_utils.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
cmd/fleet/serve.goee/server/integrationtest/condaccess/condaccess_test.goserver/fleet/mdm.goserver/datastore/mysql/conditional_access_scep_test.goserver/datastore/mysql/mysql.goserver/datastore/mysql/hosts.goee/server/integrationtest/condaccess/suite.goee/server/service/condaccess/config_test.goee/server/service/condaccess/depot/depot.goserver/datastore/mysql/conditional_access_scep.goserver/fleet/datastore.goee/server/integrationtest/condaccess/scep_rate_limit_test.goserver/mock/datastore_mock.goee/server/service/condaccess/scep.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/migrations/tables/20251103000000_AddConditionalAccessSCEPTables.goserver/service/testing_utils.goee/server/service/condaccess/config.go
🧠 Learnings (12)
📓 Common learnings
Learnt from: getvictor
Repo: fleetdm/fleet PR: 34566
File: server/service/integration_core_test.go:7500-7511
Timestamp: 2025-10-21T16:04:18.069Z
Learning: Okta conditional access app config in Fleet is Premium-gated and supported both on-prem and in Fleet Cloud; the Cloud-only enforcement applies to the Microsoft compliance partner endpoints, not to the Okta settings.
📚 Learning: 2025-07-08T16:06:54.576Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Applied to files:
cmd/fleet/serve.goee/server/service/condaccess/depot/depot.go
📚 Learning: 2025-07-08T16:12:48.797Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Applied to files:
cmd/fleet/serve.goserver/datastore/mysql/mysql.goee/server/service/condaccess/depot/depot.go
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
ee/server/integrationtest/condaccess/condaccess_test.goserver/fleet/mdm.goserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/conditional_access_scep_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/mysql.go
📚 Learning: 2025-07-08T16:11:49.555Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:115-115
Timestamp: 2025-07-08T16:11:49.555Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the error from result.RowsAffected() is intentionally ignored because the information is only used for logging purposes, not for critical program logic.
Applied to files:
server/datastore/mysql/mysql.go
📚 Learning: 2025-07-07T22:21:15.748Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.748Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
Applied to files:
server/datastore/mysql/hosts.goserver/datastore/mysql/schema.sql
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet, tests may validly use testing.T.Context() when the module/toolchain targets Go 1.24+. Do not flag t.Context() usage in this codebase if go.mod/toolchain indicates Go >= 1.24.
Applied to files:
ee/server/integrationtest/condaccess/suite.go
📚 Learning: 2025-08-01T15:08:16.858Z
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31508
File: server/datastore/mysql/schema.sql:102-116
Timestamp: 2025-08-01T15:08:16.858Z
Learning: The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations.
Applied to files:
server/datastore/mysql/schema.sql
📚 Learning: 2025-09-18T21:55:10.359Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 33184
File: server/datastore/mysql/migrations/tables/20250918154557_AddKernelHostCountsIndexForVulnQueries.go:12-27
Timestamp: 2025-09-18T21:55:10.359Z
Learning: In Fleet's codebase, schema.sql is auto-generated from all existing migrations, so there's no risk of migrations creating duplicate database objects. Migrations don't need to be made idempotent to guard against conflicts with schema.sql since they work together as part of the same system.
Applied to files:
server/datastore/mysql/schema.sql
📚 Learning: 2025-07-08T16:13:39.114Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go:53-55
Timestamp: 2025-07-08T16:13:39.114Z
Learning: In the Fleet codebase, Down migration functions are intentionally left empty/no-op. The team does not implement rollback functionality for database migrations, so empty Down_* functions in migration files are correct and should not be flagged as issues.
Applied to files:
server/datastore/mysql/migrations/tables/20251103000000_AddConditionalAccessSCEPTables.go
🔇 Additional comments (14)
server/datastore/mysql/hosts.go (1)
533-575: LGTM! Addition follows the established pattern.The addition of
"conditional_access_scep_certificates"to thehostRefsslice is consistent with the existing"host_identity_scep_certificates"entry and correctly ensures that conditional access SCEP certificate records are cleaned up when a host is deleted. ThedelHostReffunction properly filters byhost_id IN (?), satisfying the requirement for appropriate filtering criteria.cmd/fleet/serve.go (1)
1353-1361: LGTM! Conditional Access SCEP registration follows the established pattern.The implementation correctly mirrors the Host Identity SCEP setup, with proper error handling and appropriate conditional checks for private key availability and premium licensing.
ee/server/service/condaccess/config.go (1)
12-56: LGTM! Asset initialization is well-implemented and idempotent.The function correctly checks for existing assets before generating new ones, ensuring idempotency. The CA configuration (10-year validity, clear organization name) is appropriate for conditional access certificates.
server/fleet/mdm.go (1)
857-860: LGTM! Asset name constants follow the established naming convention.The new constants are appropriately named and documented, consistent with existing MDM asset definitions.
server/datastore/mysql/mysql.go (1)
187-191: LGTM! Depot factory method follows the established pattern.The implementation correctly delegates to the conditional access depot package with appropriate parameters, consistent with the Host Identity SCEP depot factory method.
server/service/testing_utils.go (1)
354-357: LGTM! Test utilities follow the established pattern.The Conditional Access test support is structured consistently with the existing Host Identity test setup, enabling integration testing of the SCEP flow.
Also applies to: 395-395, 513-515
ee/server/service/condaccess/config_test.go (1)
16-116: LGTM! Comprehensive test coverage for asset initialization.The test thoroughly validates:
- Asset creation and retrieval
- PEM encoding and X.509 certificate parsing
- Certificate properties (CA flags, key usage, validity period with leap year tolerance)
- RSA key properties (2048-bit strength)
- Idempotency (no regeneration on subsequent calls)
The use of
t.Context()is appropriate for the Go version in use.server/datastore/mysql/schema.sql (1)
266-271: Verify datetime precision inconsistency.The
not_valid_beforeandnot_valid_aftercolumns use plaindatetime(second precision), whilecreated_atandupdated_atusedatetime(6)(microsecond precision). This inconsistency within the same table could lead to confusion.Consider using consistent precision throughout the table. If the validity timestamps don't require microsecond precision, the current design is acceptable, but it would be helpful to document why different precisions are needed.
server/datastore/mysql/conditional_access_scep.go (2)
35-41: Verify rate limiting logic intentionally includes all certificates.The query returns the most recent certificate's
created_atregardless of revoked status or expiration. This likely prevents rapid re-enrollment attempts even after revocation, but please confirm this is the intended rate limiting behavior.
1-51: LGTM: Clean implementation with consistent patterns.The implementation follows Fleet's datastore patterns well with appropriate use of reader contexts, consistent error handling, and clear separation of concerns for the two lightweight read operations.
server/mock/datastore_mock.go (4)
1574-1577: New CA SCEP mock function types — LGTMSignatures look right and consistent with existing mocks.
3921-3926: DataStore fields and Invoked flags added — LGTMThread-safe flagging matches the file’s established pattern.
9385-9390: GetConditionalAccessCertCreatedAtByHostID mock — LGTMMatches established mock style; no issues.
9378-9383: Verification passed — all signatures and implementations matchThe script output confirms:
- Interface definitions exist with correct signatures
- MySQL implementations have matching signatures
- Mock implementations (lines 9378, 9385) have correct signatures matching the interface
- Call sites are present and usage is correct
No issues found. The mutex/flag/delegate pattern is correct and consistent as originally noted.
# Conflicts: # server/datastore/mysql/schema.sql
server/datastore/mysql/migrations/tables/20251106000000_AddConditionalAccessSCEPTables.go
Show resolved
Hide resolved
| // Serial allocates and returns a new (increasing) serial number. | ||
| func (d *ConditionalAccessSCEPDepot) Serial() (*big.Int, error) { | ||
| // Insert an empty row to generate a new auto-incremented serial number | ||
| result, err := d.db.Exec(`INSERT INTO conditional_access_scep_serials () VALUES ();`) |
There was a problem hiding this comment.
do we have other patterns of calling raw sql in the service layer?
There was a problem hiding this comment.
depot.go is not really a service layer, it is a datastore layer, similar to our Apple MDM SCEP
fleet/server/datastore/mysql/scep.go
Line 57 in 0180cc8
Perhaps these files should be under ee/server/condaccess and not under ee/server/service/condaccess?
The condaccess package encapsulates the SCEP logic and the IdP logic for this feature.
I recommend making any directory moves in a subsequent PR.
# Conflicts: # server/datastore/mysql/schema.sql
Related issue: Resolves #34542
Checklist for submitter
Testing
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Tests